Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: SOF: ipc4-control: Use SOF_CTRL_CMD_BINARY as numid for bytes_ext #5281

Open
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from

Conversation

ujfalusi
Copy link
Collaborator

@ujfalusi ujfalusi commented Jan 2, 2025

The header.numid is set to scontrol->comp_id in bytes_ext_get and it is
ignored during bytes_ext_put.
The use of comp_id is not quite great as it is kernel internal
identification number.

Set the header.numid to SOF_CTRL_CMD_BINARY during get and validate the
numid during put to provide consistent and compatible identification
number as IPC3.

For IPC4 existing tooling also ignored the numid but with the use of
SOF_CTRL_CMD_BINARY the different handling of the blobs can be dropped,
providing better user experience.

Reported-by: Seppo Ingalsuo [email protected]
Closes: #5282
Fixes: a062c88 ("ASoC: SOF: ipc4-control: Add support for bytes control get and put")
Cc: [email protected]
Signed-off-by: Peter Ujfalusi [email protected]

@@ -613,7 +613,11 @@ static int _sof_ipc4_bytes_ext_get(struct snd_sof_control *scontrol,
if (data_size > size)
return -ENOSPC;

header.numid = scontrol->comp_id;
/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi @singalsu is this when you do a get for reading the blob back from the DSP?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and oh the link is a SOF commit? is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi @singalsu is this when you do a get for reading the blob back from the DSP?

This is when user space asking for the binary content.
With IPC3 the numid was hardwired to SOF_CTRL_CMD_BINARY but IPC4 uses the component ID, which is more aligned with the numid.

Afaik it works correctly with the comp_id, but @singalsu is experimenting with some new way and that needs the numid to be SOF_CTRL_CMD_BINARY for some reason, I guess he can explain it here.

In any case, I will update the commit message and/or comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and oh the link is a SOF commit? is that correct?

We don't have an issue filed for this and @singalsu only mentioned it in a comment which I tried to link. I'll drop the link tag.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 The issue is that it's confusing that the get response is not the same as the data that the control was set with. Also, the get response is different from every instance of component. e.g. different instances of IIR return different header despite that if they were set up with the same blob in set.

This header wasn't earlier visible but since UCMv2 chose to use the blob format with the header (#9092) I'm now changing sof-ctl to use same format.

Copy link

@singalsu singalsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the get response to look like the data that was used to set the control.

@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4-control-constant-numid-for-bytes-ext-01 branch from 6f82180 to afa5ed6 Compare January 3, 2025 13:09
The header.numid is set to scontrol->comp_id in bytes_ext_get and it is
ignored during bytes_ext_put.
The use of comp_id is not quite great as it is kernel internal
identification number.

Set the header.numid to SOF_CTRL_CMD_BINARY during get and validate the
numid during put to provide consistent and compatible identification
number as IPC3.

For IPC4 existing tooling also ignored the numid but with the use of
SOF_CTRL_CMD_BINARY the different handling of the blobs can be dropped,
providing better user experience.

Reported-by: Seppo Ingalsuo <[email protected]>
Closes: thesofproject#5282
Fixes: a062c88 ("ASoC: SOF: ipc4-control: Add support for bytes control get and put")
Cc: [email protected]
Signed-off-by: Peter Ujfalusi <[email protected]>
@ujfalusi ujfalusi changed the title ASoC: SOF: ipc4-control: Use SOF_CTRL_CMD_BINARY as numid in bytes_ex… ASoC: SOF: ipc4-control: Use SOF_CTRL_CMD_BINARY as numid for bytes_ext Jan 3, 2025
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Jan 3, 2025

Changes since v1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The TLV header numid is ignored in bytes control set from user space and random value seen in header in get
4 participants